Rename otherwise to catch and always to finally#206
Rename otherwise to catch and always to finally#206WyriHaximus wants to merge 1 commit intoreactphp:masterfrom
Conversation
e83bdf9 to
b21fd1f
Compare
Due to limitations in the PHP language these two methods couldn't use keywords as names. With PHP 7+ this is possible, and it makes it a lot clearer what the methods do. Refs: reactphp#19
b21fd1f to
a937715
Compare
|
@WyriHaximus Thanks for looking into this, the changes makes perfect sense to me! This PR doesn't allow edits, so I've just filed #208 that builds on top of this but adds additional documentation and tests to keep 100% code coverage of the affected code paths. WDYT? |
|
@clue I'd rather have you ping me next time so I can cherry pick your commit into this PR. But this route also works for this time. Will make sure I have the allow edits by maintainers checkbox enabled. (Which is should be interestingly enough.) |
|
@WyriHaximus Fair enough, will do next time, sorry for the confusion! The diff between both PRs also turned out to be rather big (much bigger than I originally expected) due to the duplicate tests, so I figured it could make sense to take a look at both and see which approach makes most sense. In either case, thanks for kicking this off! |
|
@clue Another option would have been to take my commit and append it with your changes. But in a broader sense, it's about having everyone that worked on a feature/fix/maintenance on the changelog/release, not just the person filing the PR that got in. |
|
@WyriHaximus My bad, agree 💯 Credit where credit is due, my PR should have been two commits at the very least. I don't think there's a reasonable way to revert the merge, but happy to include you as part of the release notes for this PR! |
|
@clue I don't expect a revert, I approved and merged it myself. If I expected a revert I wouldn't have done that 😉 . But I do think we can try and see if doing this with more PR's/contributions makes sense. |
Due to limitations in the PHP language these two methods couldn't use keywords as names. With PHP 7+ this is possible, and it makes it a lot clearer what the methods do.
Refs: #19